-
Notifications
You must be signed in to change notification settings - Fork 38.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
client-gen: independent scheme for clientsets #41486
client-gen: independent scheme for clientsets #41486
Conversation
configShallowCopy.ParameterCodec = parameterCodec | ||
} | ||
if configShallowCopy.NegotiatedSerializer == nil { | ||
configShallowCopy.NegotiatedSerializer = serializer.DirectCodecFactory{CodecFactory: codecs} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've missed where codecs is coming from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
register.go in the same package.
|
||
var groupFactoryRegistry = make(announced.APIGroupFactoryRegistry) | ||
var registry = registered.NewOrDie(os.Getenv("KUBE_API_VERSIONS")) | ||
var scheme = runtime.NewScheme() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deads2k your comment from irc was that runtime.RawExtension
needs all types that should be embedded. E.g. in the PodTemplate case this is the pod.
We could make scheme
public of course, or add some way to compose apigroups and clients explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to be public or openshift will break downstream.
"k8s.io/apimachinery/pkg/apimachinery/registered" | ||
"k8s.io/apimachinery/pkg/runtime" | ||
"k8s.io/apimachinery/pkg/runtime/serializer" | ||
"k8s.io/client-go/pkg/apis/batch/install" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
your plan is to generate this and import and install all the ones we need?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a bit more thought we should be able to handle the clientsets as a big module and combine them easily. Something around these lines:
kubeclientset.AddToScheme(originclientset.Scheme)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a bit more thought we should be able to handle the clientsets as a big module and combine them easily. Something around these lines:
I agree, but I don't know know if we get there in 1.6. Think you can tie off generators for a separate scheme in 1.6?
119d47c
to
dd47c2b
Compare
@@ -60,9 +60,9 @@ func (g *genClientset) Imports(c *generator.Context) (imports []string) { | |||
for _, group := range g.groups { | |||
for _, version := range group.Versions { | |||
typedClientPath := filepath.Join(g.typedClientPath, group.Group.NonEmpty(), version.NonEmpty()) | |||
imports = append(imports, strings.ToLower(fmt.Sprintf("%s%s \"%s\"", version.NonEmpty(), group.Group.NonEmpty(), typedClientPath))) | |||
imports = append(imports, strings.ToLower(fmt.Sprintf("%s%s \"%s\"", group.Group.NonEmpty(), version.NonEmpty(), typedClientPath))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow, really? This is how its being done in client-go? I was reasonably sure you could use it from the universe and it would just end up imported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not clean for sure. Maybe somebody didn't like the autogenerated aliases.
@@ -102,6 +112,17 @@ func (g *genClientset) GenerateType(c *generator.Context, t *types.Type, w io.Wr | |||
return sw.Error() | |||
} | |||
|
|||
var globalsTemplate = ` | |||
var Scheme = $.runtimeNewScheme|raw$() | |||
var codecs = $.serializerNewCodecFactory|raw$(scheme) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capital Scheme
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fake clientsets have some issues left.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do this tomorrow.
@@ -195,26 +191,20 @@ func (c *$.GroupVersion$Client) RESTClient() $.RESTClientInterface|raw$ { | |||
|
|||
var newClientForRESTClientTemplate = ` | |||
// New creates a new $.GroupVersion$Client for the given RESTClient. | |||
func New(c $.RESTClientInterface|raw$) *$.GroupVersion$Client { | |||
return &$.GroupVersion$Client{c} | |||
func New(c $.RESTClientInterface|raw$, parameterCodec $.runtimeParameterCodec|raw$) *$.GroupVersion$Client { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be a very unpopular change. You should be able to keep the old API with a generated and working default param codec (which we'll use 99% of the time) and if you must, a separate method that can take it as a parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
dd47c2b
to
c421b89
Compare
var Codecs = serializer.NewCodecFactory(Scheme) | ||
var ParameterCodec = runtime.NewParameterCodec(Scheme) | ||
|
||
func init() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This matches my expectations.
@@ -63,26 +63,22 @@ func NewForConfigOrDie(c *rest.Config) *AutoscalingV1Client { | |||
|
|||
// New creates a new AutoscalingV1Client for the given RESTClient. | |||
func New(c rest.Interface) *AutoscalingV1Client { | |||
return &AutoscalingV1Client{c} | |||
return &AutoscalingV1Client{c, scheme.ParameterCodec} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
gv, err := schema.ParseGroupVersion("autoscaling/v1") | ||
if err != nil { | ||
return err | ||
gv := schema.GroupVersion{Group: "autoscaling", Version: "v1"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much nicer.
// if autoscaling/v1 is not enabled, return an error | ||
if !api.Registry.IsEnabledVersion(gv) { | ||
return fmt.Errorf("autoscaling/v1 is not enabled") | ||
if config.ParameterCodec == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be correct enough to auto-assign the generated one? If this doesn't cause many problems, I don't really object, but I also don't expect many people to mess with it.
return err | ||
gv := schema.GroupVersion{Group: "autoscaling", Version: "v1"} | ||
if config.NegotiatedSerializer == nil { | ||
return fmt.Errorf("expected non-nil NegotiatedSerializer for %v client", gv) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we choose our generated one by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we had the circular dependency which is gone now. Fixing.
Some comments on defaulting, but I like the output a lot better. |
c421b89
to
3f43ace
Compare
lgtm. Looks like you have a few rough edges to work out. |
3f43ace
to
f284cea
Compare
f284cea
to
4a27690
Compare
732a8ec
to
d48f390
Compare
@sttts check the staging godeps on this. |
d48f390
to
595f4ed
Compare
[APPROVALNOTIFIER] This PR is APPROVED The following people have approved this PR: smarterclayton, sttts Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
@sttts: The following test(s) failed:
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@k8s-bot verify test this |
|
@k8s-bot verify test this |
Automatic merge from submit-queue (batch tested with PRs 41146, 41486, 41482, 41538, 41784) |
This PR adds a clientset internal scheme instead of using
pkg/api.Scheme
. The clientset API stays the same.In detail:
introduce a scheme for each clientset, i.e. do not use
pkg/api.Scheme+Registry+Codec+ParameterCodecs
.This makes it easier to compose client-go's clientset (which is rewritten in
staging/copy.sh
and therefore hardcoded to usek8s.io/client-go/pkg/api.Scheme+Registry+Codecs+ParameterCodecs
) with third-party clientsets (kube-aggregator, openshift, federation) which are not rewritten usingcopy.sh
as all of them are self-contained and therefore relocatable.This fixes https://github.com/kubernetes/kubernetes/pull/41403/files#diff-76edfb07dee54ff7ddeda25c33c10d29R81 and prepares client-gen for use in OpenShift.
register types into the clientset scheme via
AddToScheme
for versioned clientsets. This decouples the client-go clients from announce+registration (internal clients continue using announce+registry and apigroup installers).This reduces complexity for client-go, possibly remove the necessity for the announce+register machinery for many use-cases, maybe even to delete it mid-term.
port federation and testgroup
install/install.go
toannounced.GroupMetaFactory
in order to have a properInstall.Install(...)
func for registration.With the first change it's easy to add the types of one clientset to the scheme of the other using the
clientset/scheme.AddToScheme
method. This allows to use cross-clientsetruntime.RawExtensions
:Kubernetes types with a
RawExtension
can en/decode aggregator types after this.TODO:
*Options
types registered correctly for core, compare DO-NOT-MERGE commit.staging/copy.sh
and run tests: TEST: update client-go with clientset specific scheme #41744[ ] fixup usage through-out the code-baseimport_known_versions.go
files somewhere such that import of theapi.Scheme
package automatically installs the apigroups. It looks like we depended on the import fo the clientset for this purpose.